Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Homogenize link style #343

Merged
merged 17 commits into from
Aug 12, 2023
Merged

Homogenize link style #343

merged 17 commits into from
Aug 12, 2023

Conversation

dchiquito
Copy link
Contributor

@dchiquito dchiquito commented Aug 4, 2023

Fixes #342

I could not find the fail2ban demo video file, so that is the only remaining dead link.

The only change I'm not confident in is https://github.com/libp2p/docs/pull/343/files#diff-0cc46df6727c5d994526392f63afa1cb76f0eec3bd2d811c83c05044a0c4bee7L189 . I couldn't find any references to "content routing", so I changed the reference to discovery routing instead.

@p-shahi p-shahi linked an issue Aug 4, 2023 that may be closed by this pull request
content/concepts/appendix/glossary.md Outdated Show resolved Hide resolved
@dchiquito
Copy link
Contributor Author

@p-shahi I converted all internal links to be of the form [foo]({{< ref "/concepts/bar.md" >}}). This means Hugo will fail to build if any links don't resolve. Sadly, it is much more verbose than a straightforward [foo](/concepts/bar). I could not find a way to get a link checker to work with internal links, so I think relying on Hugo is the best that can be done. Future writers would need to remember to adopt the more verbose link form wherever possible.

If that's not an acceptable solution, then I can do a find/replace to convert everything back to the simplest possible form. The links will be correct and the link checker will still verify external links, but there will be no CI test for internal links.

For whatever reason, neither Hugo nor any link checkers I found check heading IDs (concepts/foo#bar), so those are unverified and cannot be tested automatically AFAICT :(

@p-shahi
Copy link
Member

p-shahi commented Aug 8, 2023

@p-shahi I converted all internal links to be of the form [foo]({{< ref "/concepts/bar.md" >}}). This means Hugo will fail to build if any links don't resolve. Sadly, it is much more verbose than a straightforward [foo](/concepts/bar). I could not find a way to get a link checker to work with internal links, so I think relying on Hugo is the best that can be done. Future writers would need to remember to adopt the more verbose link form wherever possible.

If that's not an acceptable solution, then I can do a find/replace to convert everything back to the simplest possible form. The links will be correct and the link checker will still verify external links, but there will be no CI test for internal links.

For whatever reason, neither Hugo nor any link checkers I found check heading IDs (concepts/foo#bar), so those are unverified and cannot be tested automatically AFAICT :(

Thanks for the hard work @dchiquito
Having this checker in CI is great, but I agree with you that sadly it is more verbose and strays from normal markdown syntax.
I'd like to get @ElPaisano (IPFS docs maintainer) to weigh in here - wdyt?
Personally even with the added verbosity, I think this is worth it at the end of the day.

Also, I haven't looked into alternatives to lycheeverse myself, were there other alternatives you explored @dchiquito ?

@dchiquito
Copy link
Contributor Author

I looked at https://github.com/JustinBeckwith/linkinator and https://github.com/wjdp/htmltest, but lychee seemed to be the best fit and the best supported tool.

@johnnymatthews
Copy link
Contributor

Hi @dchiquito, thanks for this PR! From the looks of things, you're updating the link-checker in this repo. That might be unnecessary.

This repo is built using Hugo, which has a built-in internal link checker. By using {{< relref "/link/to/somewhere " >}} we can catch broken internal links before the site is built. We don't have to implement a link-checker CI/DI process for this.

Regarding external links: @ElPaisano has been working on a comprehensive CI/CD process for checking external links (and some other things like spell checking, and formatting). This process will likely be merged into this repo at some point soon.

I recommend that we:

  • Remove the changes to .github/workflows/link-check.yml.
  • Update all internal links to use the relref Hugo syntax

@ElPaisano
Copy link
Contributor

Hi @dchiquito, thanks for this PR! From the looks of things, you're updating the link-checker in this repo. That might be unnecessary.

This repo is built using Hugo, which has a built-in internal link checker. By using {{< relref "/link/to/somewhere " >}} we can catch broken internal links before the site is built. We don't have to implement a link-checker CI/DI process for this.

Regarding external links: @ElPaisano has been working on a comprehensive CI/CD process for checking external links (and some other things like spell checking, and formatting). This process will likely be merged into this repo at some point soon.

I recommend that we:

  • Remove the changes to .github/workflows/link-check.yml.
  • Update all internal links to use the relref Hugo syntax

@dchiquito @p-shahi I agree with Johnny's take here that we don't need to add this GH action. We can implement filecoin-project/filecoin-docs#2086 in this repo, which adds an external link check, markdown format chek, spell check and writing style check as a precommit step. Precommits can also be triggered as a GH action, so we can lint pre and post commit (if thats what the team wants)

@dchiquito thanks for updating the links! That's super useful

@ElPaisano
Copy link
Contributor

... although, looking into lycheeverse more, it might be worth using this in filecoin-project/filecoin-docs#2086, instead of https://github.com/tcort/markdown-link-check, which is what is currently used. It looks like a good tool. However, because this is a rust based tool, it won't play nice with NPM, like markdown-link-check. So that would require further refactoring

@p-shahi
Copy link
Member

p-shahi commented Aug 10, 2023

@ElPaisano will you also be porting over the work in filecoin-project/filecoin-docs#2086 to this repo?
If that's the case then I think we can close this PR

sorry for not knowing this was happening earlier @dchiquito otherwise I would have definitely let you know.

@dchiquito
Copy link
Contributor Author

@p-shahi Most of the work in this PR is finding and adjusting relative links to use Hugo's ref function. It seems like the consensus is that is a worthwhile change (or using relref, but that's a simple find/replace), so the CI changes can simply be removed and the rest of the PR merged.

@p-shahi
Copy link
Member

p-shahi commented Aug 11, 2023

That sgtm @dchiquito , we can add the filecoin-project/filecoin-docs#2086 changes later

Copy link
Member

@p-shahi p-shahi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@p-shahi p-shahi merged commit 54f96a1 into libp2p:master Aug 12, 2023
2 checks passed
@dchiquito dchiquito deleted the 342-link-style branch August 12, 2023 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Homogenize link style Audit all links
4 participants